-
Notifications
You must be signed in to change notification settings - Fork 330
Add basic support for voxel rendering and styling #1685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Update main See merge request cesium-gs/cesium-unreal!1
This comment was marked as outdated.
This comment was marked as outdated.
I could keep picking apart at this, but I think this would benefit more from a fresh pair of eyes. I'm keeping this as a draft but I would appreciate any and all reviews 🙏 |
Turns out that UE 5.4 has drastically improved the UX of Custom HLSL nodes in materials! It's now possible to define extra |
Thanks for the feedback @kring ! I agree with your points about setting up the material and generating it as a full material instead of as a layer.
Yeah, I acknowledge this system isn't great. The problem is that raymarching must be done in a |
Tried hacking a for-loop by using a couple custom nodes in a material, but it looks like Unreal doesn't like it:
Makes sense though... it'd probably end up nesting calls like |
{ | ||
%s | ||
|
||
float4 Shade(CustomShaderProperties Properties) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make this Shade function a member of CustomShaderProperties
instead? Then the properties can be accessed without qualifying them with Properties.
.
(TitleProperty = "Custom Shader", | ||
DisplayAfter = "CustomShaderPreview", | ||
MultiLine = true)) | ||
FString CustomShader = TEXT("return 1;"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest changing the default value to something like return float4(1.0, 1.0, 1.0, 0.02);
. That makes it much clearer that this is a color, and it makes the default visualization look at a bit more voxely.
FString CustomShader = TEXT("return 1;"); | ||
|
||
/** | ||
* Any additional functions to include for use in the custom shader. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Any additional functions to include for use in the custom shader. | |
* Any additional functions to include for use in the custom shader. The HLSL code provided here is included verbatim in the generated material. |
TCHAR_TO_UTF8(*dbFile)); | ||
#endif | ||
|
||
this->_pTileset->getRootTileAvailableEvent().thenImmediately([thiz = this]() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic looks like it needs to run in the main thread, so thenImmediately
is dangerous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, thiz = this
is pointless because thiz
has the same type as this
. You might as well just capture this
.
There's a bit of danger here though in that the Cesium3DTileset
could get garbage collected before the root tile is available. And I don't think there's anything that would prevent the root tile available event from still being raised. I think a simple IsValid(this)
- and early return - should be sufficient to guard against that. We do the same thing in SampleHeightMostDetailed
.
this->_pVoxelRendererComponent->UpdateTiles( | ||
pResult->tilesToRenderThisFrame, | ||
pResult->tileScreenSpaceErrorThisFrame); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A single tileset could have both voxel and non-voxel content, right? In that cause this else would be wrong, I think.
const FCesiumVoxelClassDescription* pVoxelClassDescription = | ||
this->_voxelClassDescription ? &(*this->_voxelClassDescription) : nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me why this _voxelClassDescription
is squirreled away earlier, and then accessed here. Why not get the description directly from the voxel metadata component here?
Description
Depends on CesiumGS/cesium-native#1188.
Depends on #1694, so merge that first.
This PR adds support for loading and rendering tilesets with
3DTILES_content_voxels
, parsing glTFs withEXT_primitive_voxels
payloads.Data courtesy of Swisstopo
This is a meaty set of changes, so I'll add a written walkthrough of the changes in a follow-up comment.
Author checklist
CHANGES.md
with a short summary of my change (for user-facing changes).- [ ] I have added or updated unit tests to ensure consistent code coverage as necessary.Remaining Tasks
VoxelResources
. It's a weird compromise between CesiumJS'sVoxelTraversal
and the cesium-native traversal, and it falls apart for larger datasets.Testing plan
Reach out to me offline for the pictured test data.
Cesium3DTileset
).UCesiumVoxelMetadataComponent
to gather its properties.Compatibility checks: